Skip to content

Conversation

@hugobarauna
Copy link
Member

Since filtering apps don't alter any server state, I think we can move this to the client side to improve the user experience.

Before

Simulating a 300ms latency with window.liveSocket.enableLatencySim(300)

CleanShot.2025-11-14.at.16.08.32.mp4

After

Moving to the client

CleanShot.2025-11-14.at.16.09.42.mp4

Out of curiosity, this was mostly done by Tidewave. 😎

@hugobarauna
Copy link
Member Author

One task:

I deleted the test that was verifying the behavior for that feature.

I'd like to know what approach we should follow here, since a simple liveview test isn't possible for this feature anymore.

Should we use a headless browser test?

Asking because we don't have any other type of that test yet.

@jonatanklosko ?

assert render(view) =~ slug
end

test "filter the apps based on slug, name and app folder",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's no LiveView test for that, what about adding a JS test? So we can ensure it will filter the apps based on the search criteria

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is worth adding the complexity of browser testing for this feature. One option is to use hooks (or another client-side abstraction), and unit test that instead, like we do here: https://github.com/livebook-dev/livebook/tree/main/assets/test/hooks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hooks directory actually only has unit tests for some specific encapsulated classes that we use from hooks. I don't think there's an easy way to unit test it, because it requires the page structure.

But I don't think adding browser testing for this specifically is worth it either.

Copy link
Member Author

@hugobarauna hugobarauna Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's an easy way to unit test it, because it requires the page structure.

Exactly.

But I don't think adding browser testing for this specifically is worth it either.

The /apps page is the home page for Livebook apps' users. It can be the first page internal users of Livebook app sees whenever they're using their Livebook-based internal tools.

Given that, I’d say this page is important enough to justify having end-to-end tests for it.

Copy link
Contributor

@josevalim josevalim Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then move the rendering of the apps to the client, using a simple rendering like lit? Or postpone this pull request. I am strongly against adding end-to-end tests, it will add more complexity to a test suite that is already complex because of how teams are setup, and it will impact all Livebook contributors. If this was on the teams side, I would have fewer complaints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then move the rendering of the apps to the client, using a simple rendering like lit?

I think that would add even more unnecessary complexity than having an end-to-end test.

I am strongly against adding end-to-end tests, it will add more complexity to a test suite that is already complex because of how teams are setup

I understand the cost.

One thing we can do is run those tests by default only in the CI, as we already do for other parts of the test suite, so contributors don't have to deal with them.

selectedFolder === "" || appFolderId === selectedFolder;

if (matchesSearch && matchesFolder) {
card.style.display = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use something like this.js().show(card) and this.js().hide(card) (docs), otherwise LV rerender can override the page with unfiltered state.

group.style.display = "";
const countElement = group.querySelector("[data-group-count]");
if (countElement) {
countElement.textContent = `(${count})`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, we need phx-update="ignore" on this element. And I think we also need ignore on the parent of input/select.

To test you can deploy a new app in another tab and see which state gets incorrectly reverted.

assert render(view) =~ slug
end

test "filter the apps based on slug, name and app folder",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hooks directory actually only has unit tests for some specific encapsulated classes that we use from hooks. I don't think there's an easy way to unit test it, because it requires the page structure.

But I don't think adding browser testing for this specifically is worth it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants